http: added closeAllConnections and closeIdleConnections to http.server #42812

ShogunPanda · 2022-04-21T10:46:16Z

This PR introduces two new methods on http.Server and https.Server:

closeAllConnections(): Closes all sockets connected to the server.
closeIdleConnections(): Closes all sockets connected to the server which are not actively sending a request or waiting for a response.

Fixes #41578.

nodejs-github-bot · 2022-04-21T10:46:21Z

Review requested:

@nodejs/http
@nodejs/net

doc/api/https.md

mcollina

lgtm

lpinca · 2022-04-21T12:56:08Z

For API consistency I'm -0 on this. http.Server#close(), https.Server#close(), Http2Server#close(), net.Server#close(), tls.Server#close() should all work in the same way. It might not be user friendly but developers can already do what is being proposed here by tracking connections explicitly and closing them manually.

lib/_http_server.js

mscdex · 2022-04-21T13:21:53Z

Isn't the 'keep-alive' value not very useful since non-idle keep-alive connections will still keep the process alive after their requests are finished?

ShogunPanda · 2022-04-21T13:24:36Z

For API consistency I'm -0 on this. http.Server#close(), https.Server#close(), Http2Server#close(), net.Server#close(), tls.Server#close() should all work in the same way. It might not be user friendly but developers can already do what is being proposed here by tracking connections explicitly and closing them manually.

Well, the connections list is not exposed to the developer, so they can't really do that.

I agree that all the HTTP* servers should behave the same (and it could come in a future PR for Http2Server), while net.Server and tls.Server serve a different purpose so API parity I don't think is applicable here.

ShogunPanda · 2022-04-21T13:26:45Z

Isn't the 'keep-alive' value not very useful since non-idle keep-alive connections will still keep the process alive after their requests are finished?

Actually it is bug in the implementation - Once non-idle requests have been served, they have to be evicted as well. Thanks for noticing it, I'll update this shortly.

lib/_http_server.js

lpinca · 2022-04-21T13:29:02Z

Well, the connections list is not exposed to the developer, so they can't really do that.

All servers emit the 'connection' / 'secureConnection' events. The connection list can be built by the developer using those events.

ShogunPanda · 2022-04-21T13:35:06Z

Well, the connections list is not exposed to the developer, so they can't really do that.

All servers emit the 'connection' / 'secureConnection' events. The connection list can be built by the developer using those events.

I see, make sense. I thought you were talking about the new data structure I recently introduced.

mcollina · 2022-04-21T13:38:43Z

Well, the connections list is not exposed to the developer, so they can't really do that.

All servers emit the 'connection' / 'secureConnection' events. The connection list can be built by the developer using those events.

Unfortunately that introduces a major overhead that makes it unfeasible in userland. @ShogunPanda or @jsumners could produce more evidence of this but we have researched this extensively.

lpinca · 2022-04-21T13:50:55Z

Unfortunately that introduces a major overhead that makes it unfeasible in userland. @ShogunPanda or @jsumners could produce more evidence of this but we have researched this extensively.

I'm interested because it only takes a set of references to already existing objects. I would be surprised if that turns out to be a major overhead.

jsumners · 2022-04-21T14:10:25Z

I believe this PR is a direct result of:

The primary issue that impacts performance is retaining references to the requests associated with the sockets. Ideally, this feature would let any active requests finish and then reap the remaining sockets. Currently, Node waits for all sockets to close before terminating even if there are no pending requests on those sockets.

According to information in the above links, information about pending requests is not publicly available from the socket reference. So it isn't possible to determine if any requests are pending in "user land" without also keeping track of the requests. The force close implementation in Fastify ignores this and terminates all sockets regardless of whether or not any requests are currently active. This is a poor implementation, but was the only remotely performant way to get it done.

doc/api/http.md

lpinca · 2022-04-21T14:27:35Z

According to information in the above links, information about pending requests is not publicly available from the socket reference. So it isn't possible to determine if any requests are pending in "user land" without also keeping track of the requests.

I understand but in this case the patch would only be useful if the force option is set to 'keep-alive' and there is no guarantee that the process will exit because there might be other open connections like WebSocket connections. The other option value would be equivalent to something like this:

const http = require('http');

const sockets = new Set();

const server = http.createServer();

server.on('connection', function (socket) {
  sockets.add(socket);
  socket.on('close', socketOnClose);
});

function socketOnClose() {
  sockets.delete(this);
}

server.close();

for (const socket of sockets) {
  socket.destroy();
}

ShogunPanda · 2022-04-21T14:47:24Z

@lpinca Yes, for the all case you are right. Even though taking advantage of the node internal is obviously faster than using EventEmitter and introduces less overhead.

ShogunPanda · 2022-04-21T14:48:49Z

About the WebSocket objections, you are also right. But we assume developers know what they are doing :)

The keep-alive option is useful to let current pending request to be handled but without accepting any new request on already opened connections (which is possible as of today).

ShogunPanda · 2022-04-23T07:43:12Z

As a different solution to achieve this, I might leave close as it is now and add the following new methods to http.Server (and https.Server):

server.closeConnections([type]), which performs the same new logic I just added to .close
connections, activeConnections, idleConnectins and expiredCollections, which make the various connections list readable from the developer.

The reason for 1 is to being able to solve the issue without changing existing functionality.

The reason for 2 is to enable developer to do custom connections handling without having to trace them via EE.

What do you think?

mcollina · 2022-04-23T08:21:56Z

Will exposing the connection lists be enough to implement this feature in userland?

ShogunPanda · 2022-04-23T08:30:39Z

Absolutely. The other method would just become a fancy for loop.

mcollina · 2022-04-23T11:16:06Z

Then let's expose the lists.

ShogunPanda · 2022-04-25T13:53:19Z

Done.

@nodejs/http @lpinca Can you please re-review?

jsumners · 2022-04-25T16:01:04Z

doc/api/http.md

@@ -1443,6 +1443,31 @@ This event is guaranteed to be passed an instance of the {net.Socket} class,
 a subclass of {stream.Duplex}, unless the user specifies a socket
 type other than {net.Socket}.

+### `server.activeConnections()`


A request is not a connection. A request is a communication across a connection. I think server.activeConnections is misleading. According to the subsequent docs, it is server.activeRequests.

Nope, it's connections. I return the sockets, even though the way the sockets are categorized is given by whether there is an active/expired request being sent through it.

How does this help with draining connections? The socket does not expose the requests associated with it.

I see your point now.
Let me clarify that, of all these methods, only allConnections and idleConnections are strictly needed to properly implement the forced close logic.

But I do see your point and, on a second thought, it makes sense to expose the requests rather than socket. This way the developer has all the needed informations to implement any logic they might want.

My point is that requests are typically short lived. Whereas connections can be open for literal wall clock minutes. What Node.js currently supports is:

.close stops listening for new connections but waits for current connections to end before the process can exit. This scenario allows new requests on existing connections until they timeout.

There are two other scenarios that can/should be supported:

Terminate all connections regardless of pending/active requests such that the process can terminate. This is the scenario Fastify is currently implementing.

Stop listening for new connections and stop accepting new requests while allowing current requests to finish and subsequently terminating the associated connections.

We have seen other communities provide scenario 1, so maybe 2 isn't feasible. I don't know.

#2 is hard as it needs a check every time a request is finished. Which will impact performance. So eventually (which is the intention of this PR) we go with 1 (as suggested in my last comment).

jsumners · 2022-04-25T16:02:44Z

doc/api/http.md

@@ -1453,6 +1478,18 @@ added: v0.1.90

 Stops the server from accepting new connections. See [`net.Server.close()`][].

+### `server.expiredConnections()`


I'm not clear what this provides. If a request is expired, that means Node is no longer tracking it, correct? Similarly, if a connection has expired it should have also been removed by Node, yes? So what utility does this method provide?

You are 90% right. The thing is that the Node evicts expired connection every http.Server.connectionsCheckingInterval milliseconds (default is 30s), so it can definitely happen that connections are expired but not evicted yet. Hence this method to eventually allow the user to do other things (like log these connections before evictions or similar).

doc/api/http.md

lib/_http_server.js

tniessen

Constructing potentially large lists of resources generally feels like an anti-pattern in Node.js, and unless the application code is entirely synchronous, it is prone to race conditions.

doc/api/http.md

Fixes: #41578 PR-URL: #42812 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Notable changes: * Revert_ "deps: add template for generated headers" (Daniel Bevenius) #42978 * deps: upgrade npm to 8.9.0 (npm team) #42968 * (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (Livia Medeiros) #42768 * (SEMVER-MINOR) http: added connection closing methods (Shogun) #42812 PR-URL: TBD

Notable changes: * Revert_ "deps: add template for generated headers" (Daniel Bevenius) #42978 * deps: upgrade npm to 8.9.0 (npm team) #42968 * (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (Livia Medeiros) #42768 * (SEMVER-MINOR) http: added connection closing methods (Shogun) #42812 PR-URL: #43036

Notable changes: * This release updates OpenSSL to 3.0.3. This update can be treated as a security release as the issues addressed in OpenSSL 3.0.3 slightly affect Node.js 18. See https://nodejs.org/en/blog/vulnerability/openssl-fixes-in-regular-releases-may2022/ for more information on how the May 2022 OpenSSL releases affect other Node.js release lines. * *Revert*_ "deps: add template for generated headers" (Daniel Bevenius) #42978 * deps: update archs files for quictls/openssl-3.0.3+quic (RafaelGSS) #43022 * deps: update undici to 5.2.0 (Node.js GitHub Bot) #43059 * deps: upgrade npm to 8.9.0 (npm team) #42968 * (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (LiviaMedeiros) #42768 * (SEMVER-MINOR) http: added connection closing methods (Shogun) #42812 * deps: upgrade openssl sources to quictls/openssl-3.0.3 (RafaelGSS) #43022 * doc: add LiviaMedeiros to collaborators (LiviaMedeiros) #43039 * doc: add release key for Juan Arboleda (Juan José) #42961 * (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (LiviaMedeiros) #42768 * (SEMVER-MINOR) http: added connection closing methods (Paolo Insogna) #42812 * perf_hooks: add PerformanceResourceTiming (RafaelGSS) #42725 PR-URL: #43036

OpenSSL This update can be treated as a security release as the issues addressed in OpenSSL 3.0.3 slightly affect Node.js 18. See https://nodejs.org/en/blog/vulnerability/openssl-fixes-in-regular-releases-may2022/ for more information on how the May 2022 OpenSSL releases affect other Node.js release lines. * deps: update archs files for quictls/openssl-3.0.3+quic (RafaelGSS) #43022 * deps: upgrade openssl sources to quictls/openssl-3.0.3 (RafaelGSS) #43022 Other Notable changes * *Revert*_ "deps: add template for generated headers" (Daniel Bevenius) #42978 * deps: update undici to 5.2.0 (Node.js GitHub Bot) #43059 * deps: upgrade npm to 8.9.0 (npm team) #42968 * (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (LiviaMedeiros) #42768 * (SEMVER-MINOR) http: added connection closing methods (Shogun) #42812 * doc: add LiviaMedeiros to collaborators (LiviaMedeiros) #43039 * doc: add release key for Juan Arboleda (Juan José) #42961 * (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (LiviaMedeiros) #42768 * (SEMVER-MINOR) http: added connection closing methods (Paolo Insogna) #42812 * perf_hooks: add PerformanceResourceTiming (RafaelGSS) #42725 PR-URL: #43036

OpenSSL This update can be treated as a security release as the issues addressed in OpenSSL 3.0.3 slightly affect Node.js 18. See https://nodejs.org/en/blog/vulnerability/openssl-fixes-in-regular-releases-may2022/ for more information on how the May 2022 OpenSSL releases affect other Node.js release lines. * deps: update archs files for quictls/openssl-3.0.3+quic (RafaelGSS) #43022 * deps: upgrade openssl sources to quictls/openssl-3.0.3 (RafaelGSS) #43022 Other Notable Changes * *Revert*_ "deps: add template for generated headers" (Daniel Bevenius) #42978 * deps: update undici to 5.2.0 (Node.js GitHub Bot) #43059 * deps: upgrade npm to 8.9.0 (npm team) #42968 * (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (LiviaMedeiros) #42768 * (SEMVER-MINOR) http: added connection closing methods (Shogun) #42812 * doc: add LiviaMedeiros to collaborators (LiviaMedeiros) #43039 * doc: add release key for Juan Arboleda (Juan José) #42961 * (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (LiviaMedeiros) #42768 * (SEMVER-MINOR) http: added connection closing methods (Paolo Insogna) #42812 * perf_hooks: add PerformanceResourceTiming (RafaelGSS) #42725 PR-URL: #43036

Notable changes: OpenSSL 3.0.3 This update can be treated as a security release as the issues addressed in OpenSSL 3.0.3 slightly affect Node.js 18. See https://nodejs.org/en/blog/vulnerability/openssl-fixes-in-regular-releases-may2022/ for more information on how the May 2022 OpenSSL releases affect other Node.js release lines. - deps: update archs files for quictls/openssl-3.0.3+quic (RafaelGSS) #43022 - deps: upgrade openssl sources to quictls/openssl-3.0.3 (RafaelGSS) #43022 Other notable changes: - _Revert_ "deps: add template for generated headers" (Daniel Bevenius) #42978 - deps: update undici to 5.2.0 (Node.js GitHub Bot) #43059 - deps: upgrade npm to 8.9.0 (npm team) #42968 - (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (LiviaMedeiros) #42768 - (SEMVER-MINOR) http: added connection closing methods (Shogun) #42812 - doc: add LiviaMedeiros to collaborators (LiviaMedeiros) #43039 - doc: add release key for Juan Arboleda (Juan José) #42961 - (SEMVER-MINOR) fs: add `read(buffer[, options])` versions (LiviaMedeiros) #42768 - (SEMVER-MINOR) http: added connection closing methods (Paolo Insogna) #42812 - (SEMVER-MINOR) perf_hooks: add PerformanceResourceTiming (RafaelGSS) #42725 PR-URL: #43036

juanarbol · 2022-05-30T06:33:41Z

Some parts of this rely on top of #41263, I did not find a way to rapidly backport this

ShogunPanda · 2022-05-30T07:07:24Z

@juanarbol Unfortunately I don't think it's possible without backporting both PR. The problem is that #41263 is semver-major

juanarbol · 2022-05-31T06:07:28Z

That's a shame 💔

SimenB · 2022-11-07T16:22:31Z

Sorry for posting on an old issue, but was #42812 (comment) ever done? Running git blame on the linked line of code seems to say "no" (unchanged for 14 months)

ShogunPanda · 2022-11-14T09:35:37Z

@SimenB No, it was not done yet. I'll fix it soon.

nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Apr 21, 2022

lpinca reviewed Apr 21, 2022

View reviewed changes

doc/api/https.md Outdated Show resolved Hide resolved

mcollina approved these changes Apr 21, 2022

View reviewed changes

mscdex reviewed Apr 21, 2022

View reviewed changes

lib/_http_server.js Outdated Show resolved Hide resolved

mscdex reviewed Apr 21, 2022

View reviewed changes

lib/_http_server.js Outdated Show resolved Hide resolved

jsumners reviewed Apr 21, 2022

View reviewed changes

doc/api/http.md Outdated Show resolved Hide resolved

ShogunPanda force-pushed the close-all-connections branch from 6c1388e to 8b2bdd6 Compare April 25, 2022 13:50

ShogunPanda mentioned this pull request Apr 25, 2022

Investigate flaky test-http-server-request-timeout-keepalive #42741

Open

jsumners reviewed Apr 25, 2022

View reviewed changes

lpinca reviewed Apr 25, 2022

View reviewed changes

lib/_http_server.js Outdated Show resolved Hide resolved

tniessen reviewed Apr 26, 2022

View reviewed changes

doc/api/http.md Outdated Show resolved Hide resolved

ShogunPanda deleted the close-all-connections branch May 3, 2022 12:04

This was referenced May 4, 2022

CI Reliability 2022-05-04 nodejs/reliability#271

Open

CI Reliability 2022-05-05 nodejs/reliability#272

Open

RafaelGSS pushed a commit that referenced this pull request May 10, 2022

http: added connection closing methods

2f192c4

Fixes: #41578 PR-URL: #42812 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

RafaelGSS mentioned this pull request May 10, 2022

v18.2.0 proposal #43036

Merged

juanarbol added the backport-requested-v16.x label May 30, 2022

mcollina added dont-land-on-v14.x and removed needs-ci PRs that need a full CI run. backport-requested-v16.x labels May 31, 2022

F3n67u mentioned this pull request Jun 18, 2022

Investigate flaky test-http-server-close-idle #43466

Closed

mannyistyping mentioned this pull request Jul 5, 2022

Improve drainServer for Fastify apollographql/apollo-server#5642

Closed

targos mentioned this pull request Sep 16, 2022

http: wait for pending responses in closeIdleConnections #43890

Closed

tniessen mentioned this pull request Jun 13, 2023

Provide an easy way to get all net.Server connections #48433

Closed

This comment was marked as spam.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: added closeAllConnections and closeIdleConnections to http.server #42812

http: added closeAllConnections and closeIdleConnections to http.server #42812

ShogunPanda commented Apr 21, 2022 •

edited

Loading

nodejs-github-bot commented Apr 21, 2022

mcollina left a comment

lpinca commented Apr 21, 2022

mscdex commented Apr 21, 2022 •

edited

Loading

ShogunPanda commented Apr 21, 2022

ShogunPanda commented Apr 21, 2022

lpinca commented Apr 21, 2022

ShogunPanda commented Apr 21, 2022

mcollina commented Apr 21, 2022

lpinca commented Apr 21, 2022

jsumners commented Apr 21, 2022

lpinca commented Apr 21, 2022 •

edited

Loading

ShogunPanda commented Apr 21, 2022

ShogunPanda commented Apr 21, 2022

ShogunPanda commented Apr 23, 2022

mcollina commented Apr 23, 2022

ShogunPanda commented Apr 23, 2022

mcollina commented Apr 23, 2022

ShogunPanda commented Apr 25, 2022

jsumners Apr 25, 2022

ShogunPanda Apr 26, 2022

jsumners Apr 26, 2022

ShogunPanda Apr 26, 2022

jsumners Apr 26, 2022

ShogunPanda Apr 26, 2022

jsumners Apr 25, 2022

ShogunPanda Apr 26, 2022

tniessen left a comment

juanarbol commented May 30, 2022

ShogunPanda commented May 30, 2022

juanarbol commented May 31, 2022

SimenB commented Nov 7, 2022

ShogunPanda commented Nov 14, 2022

This comment was marked as spam.

		@@ -1453,6 +1478,18 @@ added: v0.1.90

		Stops the server from accepting new connections. See [`net.Server.close()`][].

		### `server.expiredConnections()`

http: added closeAllConnections and closeIdleConnections to http.server #42812

http: added closeAllConnections and closeIdleConnections to http.server #42812

Conversation

ShogunPanda commented Apr 21, 2022 • edited Loading

nodejs-github-bot commented Apr 21, 2022

mcollina left a comment

Choose a reason for hiding this comment

lpinca commented Apr 21, 2022

mscdex commented Apr 21, 2022 • edited Loading

ShogunPanda commented Apr 21, 2022

ShogunPanda commented Apr 21, 2022

lpinca commented Apr 21, 2022

ShogunPanda commented Apr 21, 2022

mcollina commented Apr 21, 2022

lpinca commented Apr 21, 2022

jsumners commented Apr 21, 2022

lpinca commented Apr 21, 2022 • edited Loading

ShogunPanda commented Apr 21, 2022

ShogunPanda commented Apr 21, 2022

ShogunPanda commented Apr 23, 2022

mcollina commented Apr 23, 2022

ShogunPanda commented Apr 23, 2022

mcollina commented Apr 23, 2022

ShogunPanda commented Apr 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tniessen left a comment

Choose a reason for hiding this comment

juanarbol commented May 30, 2022

ShogunPanda commented May 30, 2022

juanarbol commented May 31, 2022

SimenB commented Nov 7, 2022

ShogunPanda commented Nov 14, 2022

This comment was marked as spam.

ShogunPanda commented Apr 21, 2022 •

edited

Loading

mscdex commented Apr 21, 2022 •

edited

Loading

lpinca commented Apr 21, 2022 •

edited

Loading